Skip to content

improve forceConnect pipeline #1238

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Jan 14, 2025
Merged

improve forceConnect pipeline #1238

merged 10 commits into from
Jan 14, 2025

Conversation

@sergiyvamz
Copy link
Contributor

@sergiyvamz sergiyvamz commented Jan 8, 2025 •
edited
Loading

Summary

improve forceConnect pipeline:

  • review implemented forceConnect() for existing plugins
  • keep forceConnect() for plugins related to db authentication; remove forceConnect() for other plugins
  • allows to skip a particular plugin while calling connect() or forceConnect() pipeline

Additional Reviewers

@karenc-bq
@aaronchung-bitquill
@aaron-congo

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sergiyvamz sergiyvamz added the wip Pull request that is a work in progress label Jan 8, 2025
@github-actions
Copy link

github-actions bot commented Jan 8, 2025 •
edited
Loading

Qodana Community for JVM

It seems all right 👌

No new problems were found according to the checks applied

💡 Qodana analysis was run in the pull request mode: only the changed files were checked

View the detailed Qodana report

To be able to view the detailed Qodana report, you can either:

  1. Register at Qodana Cloud and configure the action
  2. Use GitHub Code Scanning with Qodana
  3. Host Qodana report at GitHub Pages
  4. Inspect and use qodana.sarif.json (see the Qodana SARIF format for details)

To get *.log files or any other Qodana artifacts, run the action with upload-result option set to true,
so that the action will upload the files as the job artifacts:

      - name: 'Qodana Scan'
        uses: JetBrains/qodana-action@v2023.3.1
        with:
          upload-result: true
Contact Qodana team

Contact us at qodana-support@jetbrains.com

@sergiyvamz sergiyvamz removed the wip Pull request that is a work in progress label Jan 10, 2025
@sergiyvamz sergiyvamz force-pushed the force-connect-pipeline branch from a837402 to 18ecde9 Compare January 10, 2025 21:43
aaron-congo
aaron-congo reviewed Jan 10, 2025
View reviewed changes
aaron-congo
aaron-congo reviewed Jan 13, 2025
View reviewed changes
wrapper/src/main/java/software/amazon/jdbc/plugin/ConnectTimeConnectionPlugin.java Outdated

public class ConnectTimeConnectionPlugin extends AbstractConnectionPlugin {
public class ConnectTimeConnectionPlugin extends AbstractConnectionPlugin
implements AuthenticationConnectionPlugin {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my previous comment, it seems a bit misleading to label this as an Authentication plugin when it doesn't actually provide auth functionality. Can we similarly either remove this and explicitly check for this plugin in ConnectionPluginManager#makePluginChainFunc or do something like rename AuthenticationConnectionPlugin to ForceConnectPlugin?

wrapper/src/main/java/software/amazon/jdbc/plugin/dev/DeveloperConnectionPlugin.java Outdated

public class DeveloperConnectionPlugin extends AbstractConnectionPlugin implements ExceptionSimulator {
public class DeveloperConnectionPlugin extends AbstractConnectionPlugin
implements AuthenticationConnectionPlugin, ExceptionSimulator {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to my previous comment, it seems a bit misleading to label this as an Authentication plugin when it doesn't actually provide auth functionality. Can we similarly either remove this and explicitly check for this plugin in ConnectionPluginManager#makePluginChainFunc or do something like rename AuthenticationConnectionPlugin to ForceConnectPlugin

@aaron-congo
Copy link
Contributor

FastestResponseStrategyPlugin and BenchmarkPlugin still have a forceConnect method/implementation but will be excluded from the forceConnect pipeline with these changes. If they should be in the forceConnect pipeline, can we check for them in ConnectionPluginManager#makePluginChainFunc similarly to DefaultConnectionPlugin? And if they should not be in the forceConnect pipeline, can we remove their forceConnect/connectInternal methods?

@sergiyvamz sergiyvamz force-pushed the force-connect-pipeline branch from 254473d to ac548ae Compare January 14, 2025 17:45
@sergiyvamz sergiyvamz force-pushed the force-connect-pipeline branch from ac548ae to b4da891 Compare January 14, 2025 22:00
@sergiyvamz sergiyvamz merged commit b66e33d into main Jan 14, 2025
6 checks passed
@sergiyvamz sergiyvamz deleted the force-connect-pipeline branch January 14, 2025 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aaron-congo aaron-congo aaron-congo approved these changes

@aaronchung-bitquill aaronchung-bitquill aaronchung-bitquill approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sergiyvamz @aaron-congo @aaronchung-bitquill